Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add socket stat collection for systemd socket units #968

Merged
merged 1 commit into from
Jul 5, 2018
Merged

Add socket stat collection for systemd socket units #968

merged 1 commit into from
Jul 5, 2018

Conversation

xginn8
Copy link
Contributor

@xginn8 xginn8 commented Jun 7, 2018

Query dbus for each socket unit's connection statistics.

@SuperQ
Copy link
Member

SuperQ commented Jun 7, 2018

Oh, very cool!

@SuperQ SuperQ self-requested a review June 7, 2018 16:57
@SuperQ SuperQ changed the title Adding socket stat collection for systemd socket units Add socket stat collection for systemd socket units Jun 8, 2018
@SuperQ
Copy link
Member

SuperQ commented Jun 8, 2018

Would you mind adding an entry to the CHANGELOG.md file?

* [FEATURE] Add socket unit stats to systemd collector #968

discordianfish
discordianfish approved these changes Jun 8, 2018
View reviewed changes
Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general but would be great to have some tests for this.

@xginn8
Copy link
Contributor Author

xginn8 commented Jun 11, 2018

I've added the changelog entry, I'll add some tests for this feature this week.

@xginn8
Copy link
Contributor Author

xginn8 commented Jun 12, 2018

In terms of testing, reading over the coreos/dbus documentation, there's no straightforward way to set up these counters in a mocked unit: https://godoc.org/github.com/coreos/go-systemd/dbus#UnitStatus. I can probably hack something into a Property, but that seems like shoehorning something in that should not be there (https://godoc.org/github.com/coreos/go-systemd/dbus#Property). Alternatively, I can add a call to collectSockets() in TestSystemdCollectorDoesntCrash (note all of this is also true for testing the timer unit collection). What do you think, @SuperQ @discordianfish?

@discordianfish
Copy link
Member

@xginn8 Yes. let's just call collectSockets there and merge it for now.

@xginn8
Copy link
Contributor Author

xginn8 commented Jul 3, 2018

Updated as requested, @discordianfish PTAL!

@discordianfish
Copy link
Member

LGTM, @SuperQ?

@SuperQ
Copy link
Member

SuperQ commented Jul 5, 2018

Looks like it needs a rebase, but otherwise LGTM.

@xginn8
Adding socket stat collection for systemd socket units
833bee6 
Signed-off-by: xginn8 <mamcgi@gmail.com>
@xginn8
Copy link
Contributor Author

xginn8 commented Jul 5, 2018

@SuperQ the commits have been rebased. thanks for looking!

SuperQ
SuperQ approved these changes Jul 5, 2018
View reviewed changes
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SuperQ SuperQ merged commit ac5a981 into prometheus:master Jul 5, 2018
@pgier pgier mentioned this pull request Feb 6, 2019
Merged
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
@xginn8 @oblitorum
Adding socket stat collection for systemd socket units (prometheus#968)
6a7bf7a 
Signed-off-by: xginn8 <mamcgi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperQ SuperQ SuperQ approved these changes

@discordianfish discordianfish discordianfish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xginn8 @SuperQ @discordianfish